Issue 2774: Changed the implementation of isVolumeOnManagedStorage(VolumeInfo) to… by mike-tutkowski · Pull Request #2776 · apache/cloudstack

mike-tutkowski · 2018-07-26T19:02:08Z

… check if the data store in question is for primary storage

Description

In the StorageSystemDataMotionStrategy.isVolumeOnManagedStorage(VolumeInfo) method, we should have been checking if the DataStore associated with the VolumeInfo was on primary storage before checking if it is managed storage.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

GitHub Issue/PRs

Fixes: #2774

How Has This Been Tested?

Regression testing

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

rafaelweingartner · 2018-07-26T19:12:57Z

@@ -196,10 +196,16 @@ public StrategyPriority canHandle(DataObject srcData, DataObject destData) {
    }

    private boolean isVolumeOnManagedStorage(VolumeInfo volumeInfo) {


What about a unit test here?

@DaanHoogland is planning on writing a unit test for the higher-level canHandle method (to add to this PR).

yes, I am. I am struggling with the large number of injects but I think the canHandle() should be tested indeed, as the error was because the strategy returned a wrong priority from that call.

That is why we should unit test methods instead. We need to reduce the test code. Otherwise it becomes almost impossible to maintain them.

sorry, I'm missing your point

@mike-tutkowski are you going to add some unit test(s)? this is ready for merging otherwise.

@rhtyd I believe @rafaelweingartner noted (perhaps on the mailing list) that he would like to add one unit test to this PR before it is merged. Thanks

Also, I think @DaanHoogland was planning on adding a unit test, as well.

@mike-tutkowski Daan has already sent a PR to add unit test to your branch, can you merge that. See mike-tutkowski#6

@mike-tutkowski the PR I said I wanted to create some unit tests is not this one, it is the other one I worked.

borisstoyanov · 2018-07-27T07:55:28Z

@blueorangutan package

blueorangutan · 2018-07-27T07:55:52Z

@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-07-27T08:27:33Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2212

borisstoyanov · 2018-07-27T08:30:01Z

@blueorangutan test

blueorangutan · 2018-07-27T08:30:53Z

@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-07-27T18:17:52Z

Trillian test result (tid-2893)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33622 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2776-t2893-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 60 look OK, 7 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1104.89	test_privategw_acl.py
test_04_extract_template	`Failure`	128.28	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_01_secure_vm_migration	`Error`	132.72	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	132.74	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	133.75	test_vm_life_cycle.py
test_04_nonsecured_to_secured_vm_migration	`Error`	133.75	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	17.69	test_vm_life_cycle.py
test_06_download_detached_volume	`Failure`	137.48	test_volumes.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.46	test_hostha_kvm.py

borisstoyanov

LGTM, failed tests are already addressed in separate PRs.

mike-tutkowski · 2018-08-09T16:27:24Z

OK, thanks for the clarification, Rafael. I will look at Daan’s unit test today. On Aug 9, 2018, at 5:09 AM, Rafael Weingärtner <notifications@github.com<mailto:notifications@github.com>> wrote: @rafaelweingartner commented on this pull request.

________________________________ In engine/storage/datamotion/src/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java<#2776 (comment)>:

@@ -196,10 +196,16 @@ public StrategyPriority canHandle(DataObject srcData, DataObject destData) {

} private boolean isVolumeOnManagedStorage(VolumeInfo volumeInfo) { @mike-tutkowski<https://github.com/mike-tutkowski> the PR I said I wanted to create some unit tests is not this one, it is the other one I worked. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2776 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AC4SH5gYYsN-hRc7V2cgpLyM9rzQUP3Kks5uPBgsgaJpZM4ViYUg>.

… check if the data store in question is for primary storage (and added a unit test from Daan Hoogland)

mike-tutkowski · 2018-08-10T21:07:15Z

Two LGTMs and tests look good. Merging this into 4.11.

mike-tutkowski · 2018-08-10T21:08:16Z

@DaanHoogland Do you know what process I should follow to merge this forward into master? Thanks

DaanHoogland · 2018-08-11T15:14:44Z

there is a tool in tools/git called git-fwd-merge (i think). You should use that to merge 4.11 on master and than push it to origin. Make sure you have pulled both branches first, @mike-tutkowski ;)

mike-tutkowski · 2018-08-12T06:12:20Z

@DaanHoogland I performed the merge to master from 4.11. Can you take a look at 46c56ea and double check that I did it properly? Thanks!

mike-tutkowski · 2018-08-20T18:15:53Z

@DaanHoogland Just looking at how a couple other 4.11-to-4.12 merges were handled, it looks like I did the right thing with my merge.

DaanHoogland · 2018-08-21T08:03:17Z

ok, ignoring then ... ;)

rafaelweingartner reviewed Jul 26, 2018

View reviewed changes

borisstoyanov approved these changes Jul 30, 2018

View reviewed changes

yadvr added this to the 4.11.2.0 milestone Aug 7, 2018

yadvr added the component:managed-storage label Aug 8, 2018

yadvr approved these changes Aug 8, 2018

View reviewed changes

yadvr mentioned this pull request Aug 8, 2018

unit test for secondary target mike-tutkowski/cloudstack#6

Merged

mike-tutkowski force-pushed the vol-on-managed-storage branch 3 times, most recently from d937cbf to 0378f84 Compare August 10, 2018 17:05

Changed the implementation of isVolumeOnManagedStorage(VolumeInfo) to…

ab83c19

… check if the data store in question is for primary storage (and added a unit test from Daan Hoogland)

mike-tutkowski force-pushed the vol-on-managed-storage branch from 0378f84 to ab83c19 Compare August 10, 2018 17:24

mike-tutkowski merged commit e4ec123 into apache:4.11 Aug 10, 2018

mike-tutkowski deleted the vol-on-managed-storage branch August 12, 2018 05:38

		@@ -196,10 +196,16 @@ public StrategyPriority canHandle(DataObject srcData, DataObject destData) {
		}

		private boolean isVolumeOnManagedStorage(VolumeInfo volumeInfo) {

Conversation

mike-tutkowski commented Jul 26, 2018

Description

Types of changes

GitHub Issue/PRs

How Has This Been Tested?

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisstoyanov commented Jul 27, 2018

Uh oh!

blueorangutan commented Jul 27, 2018

Uh oh!

blueorangutan commented Jul 27, 2018

Uh oh!

borisstoyanov commented Jul 27, 2018

Uh oh!

blueorangutan commented Jul 27, 2018

Uh oh!

blueorangutan commented Jul 27, 2018

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski commented Aug 9, 2018 via email

Uh oh!

mike-tutkowski commented Aug 10, 2018

Uh oh!

mike-tutkowski commented Aug 10, 2018

Uh oh!

DaanHoogland commented Aug 11, 2018

Uh oh!

mike-tutkowski commented Aug 12, 2018

Uh oh!

mike-tutkowski commented Aug 20, 2018

Uh oh!

DaanHoogland commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants